ACCUMULO-3602 BatchScanner optimization for AccumuloInputFormat by echeipesh · Pull Request #25 · apache/accumulo

echeipesh · 2015-03-31T22:56:48Z

No description provided.

…Format

…able

also: killing dead code for “iterators”

asfbot · 2015-03-31T23:08:16Z

Accumulo-Pull-Requests #21 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-04-01T15:23:01Z

Accumulo-Pull-Requests #22 SUCCESS
This pull request looks good

…MULO-3602 Conflicts: core/src/main/java/org/apache/accumulo/core/client/mapred/AbstractInputFormat.java core/src/main/java/org/apache/accumulo/core/client/mapreduce/AbstractInputFormat.java core/src/main/java/org/apache/accumulo/core/client/mapreduce/RangeInputSplit.java

asfbot · 2015-04-01T18:57:20Z

Accumulo-Pull-Requests #23 FAILURE
Looks like there's a problem with this pull request

joshelser · 2015-04-01T19:04:29Z

Ack, I'm sorry, @echeipesh, I just broke you. RangeInputSplit.getInstance() is nonsensical. It should use the ClientConfiguration from the Job's configuration (which can only be gotten by the RecordReader using the TaskAttemptContext). LMK if that doesn't make sense and I can send you a patch to fix your branch.

echeipesh · 2015-04-01T19:06:04Z

No worries, I was just looking through this, I see what happened.

asfbot · 2015-04-01T19:48:42Z

Accumulo-Pull-Requests #24 SUCCESS
This pull request looks good

joshelser · 2015-04-02T02:16:18Z

core/src/main/java/org/apache/accumulo/core/client/mapred/AbstractInputFormat.java

Duplicated call to scanner.iterator(). Looks like you do it down below too?

echeipesh · 2015-04-16T15:06:27Z

Summary of changes since last burst of comments:

Removed AccumuloInputSplit references in public API via method presented
Removed .setBatchScanThreads from configuration
Added .setBatchScan option to mapred InputFormat
Added better scaladoc for .setBatchScan
Covered all changes with @depricated since and @since tags
Made small improvement to a test to verify resumption of success after testing failure.

asfbot · 2015-04-16T15:18:52Z

Accumulo-Pull-Requests #43 SUCCESS
This pull request looks good

joshelser · 2015-04-16T22:10:42Z

core/src/main/java/org/apache/accumulo/core/client/mapred/InputFormatBase.java

I'm not seeing where this is implemented. Am I just blind?

n/m, pulled it into Eclipse and realized it's just me :)

joshelser · 2015-04-16T22:46:59Z

Ok, two things I think need to be fixed. Everything else you fixed looks good!

Restore the setupIterators(TaskAttemptContext, Scanner, RangeInputSplit) method (this is another one of those crappy edge cases, sorry).
Fail if batchScan==true while autoAdjust==true

I pulled down the PR and ran it through japi which is how I noticed the first point. Otherwise it looks good from a compatibility sense for 1.6->1.7.

Seeing it in Eclipse brought up some checkstyle warnings still (unused imports). If you want to clean those up, that'd be great; otherwise, whomever merges can probably kill them all at once.

keith-turner · 2015-04-17T00:03:12Z

I think this looks good. I am finished looking at it. I also think adding the autoAdjust sanity check and stating both are not supported in javadoc would be nice.

asfbot · 2015-04-17T16:02:22Z

Accumulo-Pull-Requests #47 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-04-17T16:09:21Z

Accumulo-Pull-Requests #48 FAILURE
Looks like there's a problem with this pull request

joshelser · 2015-04-17T16:13:14Z

👍 from me

keith-turner · 2015-04-17T16:16:41Z

+1

asfbot · 2015-04-17T16:19:37Z

Accumulo-Pull-Requests #49 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-04-17T16:52:10Z

Accumulo-Pull-Requests #51 FAILURE
Looks like there's a problem with this pull request

asfbot · 2015-04-17T17:19:08Z

Accumulo-Pull-Requests #52 SUCCESS
This pull request looks good

joshelser · 2015-04-17T17:27:31Z

Hot diggity. I'll see if I can bring this down, run some of the UTs/ITs, squash the commits, and push it up. Thanks so much for your hard work, @echeipesh!!

joshelser · 2015-04-17T19:45:16Z

Actually, compat is still broken with that setupIterators method. Do you want to fix that @echeipesh or should I just do when I apply the rest of your changes?

asfbot · 2015-04-17T20:31:39Z

Accumulo-Pull-Requests #54 SUCCESS
This pull request looks good

joshelser · 2015-04-17T21:15:27Z

mvn verify -Psunny came back positive locally. Committed as 956a50e. Thanks for all of your hard work, @echeipesh!!

joshelser · 2015-04-17T21:21:34Z

And, if you can @echeipesh, can you please close this? Because I squashed and didn't include the magic github markdown, I can't close the PR myself.

echeipesh · 2015-04-18T01:02:42Z

@joshelser, awesome! Thank you for all the help guiding this along.

…imeout (apache#25)

echeipesh added 14 commits March 30, 2015 14:13

Added support for grouping ranges per tablet when using AccumuloInput…

8da5933

…Format

Avoid casts by using AccumuloInputSplit interface

47a2fae

upgrade warning to exception if batch scan is requested but not avail…

b4101a3

…able

close underlying scanner with the RecordReader

ad83769

Upgrade AccumuloInputSplit to abstract class for code reuse

46a45c3

merging upstream master

7a09974

fix merge errors

41e79cf

add batch scan setters/getters to AccumuloInputFormat

7b3beeb

fix casting error

c84c8c0

make findbugs happy

3549514

test BatchInputSplit generation

7ac1cef

match code flow between mapred and mapreduce

9a47722

also: killing dead code for “iterators”

test all splits for correct type, not just head

23f6d40

Test MR job with BatchScan option enable

17fa276

remove unused import

4614efe

fix: broken merge, getting instance from job context

bc2a8ef

joshelser reviewed Apr 2, 2015
View reviewed changes

joshelser reviewed Apr 16, 2015
View reviewed changes

Added mention and a warning about AutoAdjustRanges

4409db9

Added warning to AutoAdjustRanges and upgraded to exception

e86541a

fix: missing semicolons

79af3e5

moving check down to after autoAdjust config is read

964bf3d

tailing whitespace

c264325

re-adding setupIterators overload for 1.6 compatability

a1c59a9

echeipesh closed this Apr 18, 2015

karthick-rn mentioned this pull request Apr 1, 2020

Accumulo master hangs after TLS on ZK #1578

Closed

keith-turner pushed a commit to keith-turner/accumulo that referenced this pull request May 25, 2022

Added metrics for Scans, to include a metric for the ScanServer busyT…

82cef0b

…imeout (apache#25)

keith-turner pushed a commit to keith-turner/accumulo that referenced this pull request May 27, 2022

Added metrics for Scans, to include a metric for the ScanServer busyT…

8e68e3d

…imeout (apache#25)

Conversation

echeipesh commented Mar 31, 2015

Uh oh!

asfbot commented Mar 31, 2015

Uh oh!

asfbot commented Apr 1, 2015

Uh oh!

asfbot commented Apr 1, 2015

Uh oh!

joshelser commented Apr 1, 2015

Uh oh!

echeipesh commented Apr 1, 2015

Uh oh!

asfbot commented Apr 1, 2015

Uh oh!

joshelser Apr 2, 2015

Choose a reason for hiding this comment

Uh oh!

echeipesh commented Apr 16, 2015

Uh oh!

asfbot commented Apr 16, 2015

Uh oh!

joshelser Apr 16, 2015

Choose a reason for hiding this comment

Uh oh!

joshelser Apr 16, 2015

Choose a reason for hiding this comment

Uh oh!

joshelser commented Apr 16, 2015

Uh oh!

keith-turner commented Apr 17, 2015

Uh oh!

asfbot commented Apr 17, 2015

Uh oh!

asfbot commented Apr 17, 2015

Uh oh!

joshelser commented Apr 17, 2015

Uh oh!

keith-turner commented Apr 17, 2015

Uh oh!

asfbot commented Apr 17, 2015

Uh oh!

asfbot commented Apr 17, 2015

Uh oh!

asfbot commented Apr 17, 2015

Uh oh!

joshelser commented Apr 17, 2015

Uh oh!

joshelser commented Apr 17, 2015

Uh oh!

asfbot commented Apr 17, 2015

Uh oh!

joshelser commented Apr 17, 2015

Uh oh!

joshelser commented Apr 17, 2015

Uh oh!

echeipesh commented Apr 18, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants